Skip to content

Generic/Syntax: add support for inspecting code passed via STDIN #1151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jul 4, 2025
Merged

Generic/Syntax: add support for inspecting code passed via STDIN #1151

merged 4 commits into from
Jul 4, 2025
+140 −4

Conversation

rodrigoprimo
Copy link
Contributor

Description

This PR improves the Generic.PHP.Syntax sniff to make it work when code is passed via STDIN. Before, any code passed this way would cause a false negative as the sniff was not taking into account that $phpcsFile->getFilename() might return STDIN instead of an actual file name.

Refer to #915 for additional context and testing instructions.

Suggested changelog entry

Fixed: the Generic.PHP.Syntax sniff now handles code passed via STDIN correctly

Related issues/external references

Fixes #915

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

@jrfnl
Copy link
Member

jrfnl commented Jun 27, 2025

@rodrigoprimo I believe there is still something wrong in the command as the tests do not pass on Windows.

Aside from that, could you please rebase this branch on the current master ?

@rodrigoprimo
Generic/Syntax: add support for inspecting code passed via STDIN
25ad6e2 
This commit improves the Generic.PHP.Syntax sniff to make it work when
code is passed via STDIN. Before, any code passed this way would cause a
false negative as the sniff was not taking into account that
`$phpcsFile->getFilename()` might return `STDIN` instead of an actual
file name.
@rodrigoprimo rodrigoprimo force-pushed the syntax-sniff-support-stdin branch from b9bb2dc to 25ad6e2 Compare June 27, 2025 12:16
@rodrigoprimo
Copy link
Contributor Author

Aside from that, could you please rebase this branch on the current master ?

I just rebased without changes, and now remark is passing.

@rodrigoprimo I believe there is still something wrong in the command as the tests do not pass on Windows.

I will take a look to understand why the tests are failing on Windows. When I was creating this PR, I checked that the Quicktest workflow was passing, but I completely forgot that the full test suite is not executed on Windows in this workflow.

@jrfnl
Copy link
Member

jrfnl commented Jun 27, 2025

@rodrigoprimo Yes, you will probably want to add @group Windows to the test you added.

@rodrigoprimo
Copy link
Contributor Author

@jrfnl, I added the @group Windows to the new test and fixed the issue on Windows.

As far as I could research, it is not straightforward to reliably pipe commands and escape characters when echoing on Windows, so I opted to create a temporary file instead when on this OS, and the file content is passed via STDIN. What do you think of this approach?

@jrfnl
Copy link
Member

jrfnl commented Jul 2, 2025

I added the @group Windows to the new test and fixed the issue on Windows.

As far as I could research, it is not straightforward to reliably pipe commands and escape characters when echoing on Windows, so I opted to create a temporary file instead when on this OS, and the file content is passed via STDIN. What do you think of this approach?

I've been thinking this over and while I appreciate all the work you've put into this, I have a feeling the Windows vs STDIN issue is theoretical only.

PHPCS doesn't currently support STDIN on Windows:

PHP_CodeSniffer/src/Config.php

Lines 432 to 436 in 368817d

if (defined('STDIN') === false
|| stripos(PHP_OS, 'WIN') === 0
) {
return;
}

Now, whether that is something which should be fixed or not is outside the scope of this issue.

But knowing that STDIN is not supported for Windows, the sniff will never be called with STDIN when running on Windows, so the issue with the STDIN test not passing on Windows should probably be fixed by a @requires OS ^(?!WIN).* instead of changing the sniff code to accommodate Windows.

What do you think ?

@jrfnl jrfnl added this to the 3.13.3 milestone Jul 2, 2025
@rodrigoprimo rodrigoprimo force-pushed the syntax-sniff-support-stdin branch from b2b0d85 to d19a939 Compare July 3, 2025 18:41
@rodrigoprimo
Copy link
Contributor Author

Good point, I failed to consider that PHPCS currently doesn't support STDIN on Windows. I went ahead, kept the original commit, dropped the two that I had added to make the sniff work when handling STDIN on Windows, and added a new one to skip the STDIN test on this OS.

jrfnl
jrfnl reviewed Jul 4, 2025
View reviewed changes
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rodrigoprimo Thanks for updating this PR. Looks good to me now. I've only got a few small nitpicks, but other that, this is good to go.

rodrigoprimo and others added 2 commits July 4, 2025 09:54
@rodrigoprimo rodrigoprimo force-pushed the syntax-sniff-support-stdin branch from dd44013 to 13c2a9a Compare July 4, 2025 12:59
@rodrigoprimo
Copy link
Contributor Author

Thanks for the final remarks, @jrfnl. They are addressed now.

jrfnl
jrfnl approved these changes Jul 4, 2025
View reviewed changes
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these updates @rodrigoprimo ! LGTM. Will merge once the build has passed.

@jrfnl jrfnl merged commit 83312f6 into PHPCSStandards:master Jul 4, 2025
48 checks passed
@jrfnl jrfnl deleted the syntax-sniff-support-stdin branch July 4, 2025 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrfnl jrfnl jrfnl approved these changes

Assignees
No one assigned
Labels
Standard: Generic Type: bug
Projects
None yet
Milestone
3.13.3
Development

Successfully merging this pull request may close these issues.

Generic.PHP.Syntax false negative when file content passed via STDIN
2 participants
@rodrigoprimo @jrfnl